Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Role aware container names #99

Merged
merged 20 commits into from
Mar 24, 2023
Merged

Role aware container names #99

merged 20 commits into from
Mar 24, 2023

Conversation

tbuehlmann
Copy link
Contributor

@tbuehlmann tbuehlmann commented Mar 10, 2023

Container names don't include their role (web, job, …), so there are naming collisions when the same service tries to run with different roles on the same host. See #44.

This doesn't work right now:

# config/deploy.yml
service: app

servers:
  web:
    - 1.1.1.1
  job:
    hosts:
      - 1.1.1.1
    cmd: bin/jobs

… as the container name would be app-<version> for web and job.

This PR solves this by adding the role to container names. The containers for the above config would be named app-web-<version> and app-job-<version>.

It also makes changes to --hosts and --roles behaviour as to how I understand it should work:

  • --hosts filters the hosts for the given command and sets MRKS's @specific_hosts
  • --roles filters the roles for the given command and sets MRKS's @specific_roles
  • MRSK.hosts takes the specific hosts (or all hosts) and filters by the specific roles
  • MRSK.roles takes the specific roles (or all roles) and filters by the specific hosts
  • Overriding hosts to values other than those mentioned in the config doesn't work anymore. Not sure this is correct but it didn't make sense to me.

Note: There's no backwards compatibility. Probably best to stop and remove all running app containers, then update mrsk, then redeploy.

TODOs:

  • Rolify Mrsk::Commands::Accessory (nothing todo)
  • Rolify Mrsk::Commands::App
  • Rolify Mrsk::Commands::Auditor
  • Rolify Mrsk::Commands::Healthcheck (nothing todo)
  • Rolify Mrsk::Commands::Prune (nothing todo)
  • Rolify Mrsk::Commands::Registry (nothing todo)
  • Rolify Mrsk::Commands::Traefik (nothing todo)
  • Non-web containers shouldn't expose 3000 so traefik doesn't get confused

Non-web containers shouldn't expose 3000 so traefik doesn't get confused

When testing on a remote server, I had the issue that web (puma) and job (sidekiq) would both be recognized by traefik while only the web container would respond to http requests and somehow no requests would go through. I initially thought exposing port 3000 would be the issue but it's the traefik labels. Moving them to only the web role resolves the issue.

@tbuehlmann tbuehlmann marked this pull request as ready for review March 10, 2023 08:52
@tbuehlmann tbuehlmann marked this pull request as draft March 10, 2023 09:58
@tbuehlmann tbuehlmann marked this pull request as ready for review March 10, 2023 11:07
@dhh
Copy link
Member

dhh commented Mar 13, 2023

Excellent work on this! Big change, so might take a couple of days before I'm able to fully review.

dhh and others added 11 commits March 14, 2023 19:11
* main:
  Add another assertion for `escape_shell_value`
  Add tests for `Mrsk::Utils`
  Fix indentation
  Don't report exception here too
  Don't report exception
  Add CLI tests for remaining commands that are not tested yet
  Minor: Properly require active_support
* main:
  Ask for access token
  Style
  Style
  config.traefik is already nil safe
  Update README.md
  Bump dev deps and consolidate platform matches
  Deploys mention the released service@version
  Accessories aren't required to publish a port
  Accessories may be pulled from authenticated registries
  Polish destination config loading
  Allow arbitrary docker options for traefik
  Fixed typos
  Fixed readme
  Rebased on main
  Added volume configuration in response to issue coments
  Modified in response to PR comments
  Added the additional_ports configuration
* main:
  Wording
  Remove accessory images using tags rather than labels
  Update readme to point to ghcr.io/mrsked/mrsk
  Validate that all roles have hosts
  Commander needn't accumulate configuration
  Pull latest image tag, so we can identity it
  Default to deploying the config version
  Remove unneeded Dockerfile.dind, update Readme
  add D-in-D dockerfile, update Readme
@dhh dhh merged commit 1f19604 into basecamp:main Mar 24, 2023
@djmb djmb mentioned this pull request Apr 12, 2023
ncreuschling pushed a commit to ncreuschling/mrsk that referenced this pull request Apr 12, 2023
Rollbacks stopped working after basecamp#99.

We'll confirm that a container is available for the first role on the
primary host before attempting to rollback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants